-
Notifications
You must be signed in to change notification settings - Fork 696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #3875. Add left and right tab sides with alignments to the TabView. #3876
base: v2_develop
Are you sure you want to change the base?
Fixes #3875. Add left and right tab sides with alignments to the TabView. #3876
Conversation
…rks on alignments.
…iew-left-right-feature
@tig there are 4 unit tests failing because the Add_Three_TabsSide_Left_ShowInitialLine_False_ChangesTab_Height5
Expected:
───────────────────┐
Tab1 hi │
────╮ │
Tab2│ │
────▼──────────────┘
But Was:
───────────────────┐
Tab1 hi │
────╮ │
Tab2│ │
────╯──────────────┘
Add_Three_TabsSide_Left_ShowInitialLine_True_ChangesTab_Height5
Expected:
╭──────────────────┐
│Tab1 hi │
├────╮ │
│Tab2│ │
╰────▼─────────────┘
But Was:
╭──────────────────┐
│Tab1 hi │
├────╮ │
│Tab2│ │
╰────╯─────────────┘
Add_Three_TabsSide_Right_ShowInitialLine_False_ChangesTab_Height5
Expected:
┌───────────────────
│hi Tab1
│ ╭────
│ │Tab2
└──────────────▼────
But Was:
┌───────────────────
│hi Tab1
│ ╭────
│ │Tab2
└──────────────╰────
Add_Three_TabsSide_Right_ShowInitialLine_True_ChangesTab_Height5
Expected:
┌──────────────────╮
│hi Tab1│
│ ╭────┤
│ │Tab2│
└─────────────▼────╯
But Was:
┌──────────────────╮
│hi Tab1│
│ ╭────┤
│ │Tab2│
└─────────────╰────╯ The strange is if there isn't enough space to add another tab an vertical line is added and the down arrow is draw. This issue only happens if the height fit exactly the tabs than can be currently visible.
|
It was a bug with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you rename the 'RenderTabLine` methods, I'll merge this.
But it doesn't make me happy that you've taken an already complex beast and made it more complex just because it would be cool to have tabs on the sides.
private void Tab_DisplayTextChanged (object? sender, EventArgs e) { _tabLocations = CalculateViewport (Viewport); } | ||
|
||
/// <summary>Renders the line with the tab names in it.</summary> | ||
private void RenderTabLine (Tab []? tabLocations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this method is just mis-named causing me to think you were doing drawing w/in Layout.
} | ||
|
||
LineCanvas.Merge (lc); | ||
} | ||
|
||
/// <summary>Renders the line of the tab that adjoins the content of the tab.</summary> | ||
private void RenderUnderline () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this method to something that actually describes what it does.
} | ||
|
||
LineCanvas.Merge (lc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code is insane.
1000 lines of highly repetitive logic that could be replaced with just a few lines if this were completed:
I get annoyed that you continue to expand on this vs. helping me think through how to make #3407 really work well.
Fixes
Proposed Changes/Todos
Pull Request checklist:
CTRL-K-D
to automatically reformat your files before committing.dotnet test
before commit///
style comments)